Codecov Report

Attention: Patch coverage is 70.00000% with 6 lines in your changes missing coverage. Please review.

Very clean PR and nice tests. Only comment/question is that TruncationComposition is a generic name that does not necessarily communicate the & (intersect) nature of the composition. Would there ever be a need for an | (union) form of truncation?

Very clean PR and nice tests. Only comment/question is that TruncationComposition is a generic name that does not necessarily communicate the & (intersect) nature of the composition. Would there ever be a need for an | (union) form of truncation?

Good question. I actually chose that name because at first I had TruncationComposition store a function that would be used to combine the spectra, where instersect was just a special case, but then I couldn't come up with a better name for the special case of intersection.

A use case I could think of for union would be filtering both large and small singular values/eigenvalues (it sounds a bit contrived but it comes up when diagonalizing correlation matrices if you want to filter the most occupied/unoccupied orbitals).

Any suggestions for other names? Maybe TruncationIntersection? Otherwise I could change the design to:

struct TruncationComposition{F,T<:Tuple{Vararg{TruncationStrategy}}} <: TruncationStrategy
    f::F
    components::T
end

with a definition:

function Base.:&(trunc1::TruncationStrategy, trunc2::TruncationStrategy)
    return TruncationComposition(intersect, (trunc1, trunc2))
end

function findtruncated(values::AbstractVector, strategy::TruncationComposition)
    inds = map(Base.Fix1(findtruncated, values), strategy.components)
    return strategy.f(inds...)
end

TensorKit had MultipleTruncation, but I'm not sure if that's any better. I think I like TruncationIntersection, seems to indicate well what it is doing, and from the user side you probably don't have to interact too much with that name either, because & is more easily accessible?

The proposed general TruncationComposition also looks fine to me, it might be "overengineered" in the sense that I can only come up with union or intersect, but probably that's also just my lack of imagination, so I don't see why not

TensorKit had MultipleTruncation, but I'm not sure if that's any better. I think I like TruncationIntersection, seems to indicate well what it is doing, and from the user side you probably don't have to interact too much with that name either, because & is more easily accessible?

Yes, I would picture these types would be internal (maybe we would use them internally in ITensor to implement some fancier truncation schemes, though even in those cases I imagine & will suffice).

The proposed general TruncationComposition also looks fine to me, it might be "overengineered" in the sense that I can only come up with union or intersect, but probably that's also just my lack of imagination, so I don't see why not

I guess I could imagine other set operations (setdiff, symdiff) could be useful.

For now I'll go with TruncationIntersection, and we can consider the more general TruncationComposition design if we start needing other compositions and don't want to define types for each one. TruncationIntersection could later be defined as TruncationComposition{typeof(intersect)}.

Looks good to me. I also liked the TruncationComposition proposal with the f field. You then only loose that you cannot join more than 2 components as you might have intersections and unions. So in that case I would just have 2 component fields explicitly, instead of a tuple field, and also nest TruncationComposition objects (as in reality there will not be that many of them I believe).

But the current TruncationIntersection is good. Shall I merge?

Looks good to me. I also liked the TruncationComposition proposal with the f field. You then only loose that you cannot join more than 2 components as you might have intersections and unions. So in that case I would just have 2 component fields explicitly, instead of a tuple field, and also nest TruncationComposition objects (as in reality there will not be that many of them I believe).

Yes, that would be something we'd need to work out. I was thinking we could still have a tuple field, but if the fs are equal we could splat them together, and otherwise it would become nested. (But as you say just making them nested in general would probably be fine.)

But the current TruncationIntersection is good. Shall I merge?

I'm good with this being merged. I'd be happy to implement the more general TruncationComposition in a future PR when use cases arise.